At first I misunderstood "onTokenRefresh never fires" to mean that there isn't a possible case where the FCM system decides to replace the token during an app session (resulting in an onTokenRefresh call).

I think what you mean (since you do mention that case in an implementation comment in _onTokenRefresh) is that an onTokenRefresh call isn't part of the startup sequence except on the first startup after install.

I wonder if my misreading started with interpreting "on a typical launch of the app" instead as "in a session that started with a typical launch". Anyway, I wonder if there's an alternative wording that helps prevent a misreading like mine?

Hmm, not quite that either — here's another way of putting what I mean here:

In a typical session of the app, onTokenRefresh never fires and our _onTokenRefresh callback is never called. In that case this code is the only way we learn the token value in that session.

That's "typical" in that the exceptions are the first time the app runs after install, and any time the token changes while the app is running. (And the latter is expected to be rare.)

The real point, though, is just that there are some cases where onTokenRefresh never fires and so we have to rely on this return value from getToken. As it happens, they're not even an edge case; they're the usual case.

Does the token-registration call want to go earlier than this, which comes after /register? For some users, /register will regularly take several seconds.

In particular, for the case where the user has already been getting and expecting notifications, if the device token has changed since the last run of the app, we have kind of an urgent task to try to register the new token so we don't unnecessarily drop notifications. (I see zulip-mobile doesn't do this earlier than /register success, but I wonder if that needs to be carried over to zulip-flutter.)

I see that we don't have our hands on a LivePerAccountStore (to call its registerNotificationToken) until after /register succeeds…hmm. But as a comment on LivePerAccountStore.load says, we might someday load a LivePerAccountStore from local storage, and that would let us call a LivePerAccountStore.registerNotificationToken sooner than the /register request.

…But actually, does the job of registerNotificationToken really need to wait until we have a LivePerAccountStore? (Does it need to be a method on LivePerAccountStore?) As implemented, it looks like its only dependencies are the NotificationService instance and an ApiConnection. The former is global, and for the latter we already have the value we need before starting the /register request. We may in the future need an ackedPushToken, to condition on whether the new token matches the old. But we can get that from the Account we already have our hands on before starting the /register request.

As implemented, it looks like its only dependencies are […]

I guess it rightly has another dependency: some means of tracking when the listener it adds on NotificationService.instance.token needs to be removed. That's a convenient role for LivePerAccountStore to fill…hmm. I see the "TODO call removeListener on [dispose]" in registerNotificationToken.

Does the token-registration call want to go earlier than this, which comes after /register?

Hmm, good thought. I think I basically just copied this aspect from zulip-mobile.

I guess it rightly has another dependency: some means of tracking when the listener it adds on NotificationService.instance.token needs to be removed. That's a convenient role for LivePerAccountStore to fill…hmm. I see the "TODO call removeListener on [dispose]" in registerNotificationToken.

Yeah. But that's really just because on dispose, we may be about to make a new LivePerAccountStore for the same account — so it's a way of preventing duplication. If the listener lived somewhere else that had a different lifetime, then we'd want to cancel it based on that thing's lifetime instead.

I guess there's the complication (which we'll eventually implement) that the user's API key could become invalid, or the user or realm get deactivated. In that case we'll want to tear down whatever has that listener, along with the LivePerAccountStore and/or whatever's doing the polling for events.

…But actually, does the job of registerNotificationToken really need to wait until we have a LivePerAccountStore? (Does it need to be a method on LivePerAccountStore?)

Yeah, I don't totally love having it here. It's here because it seems parallel to the event-polling loop poll; and because this is the most obvious place for the code to live if it's going to be initiated after we get the register response, i.e. from load.

Ah interesting; I guess this early return doesn't make things fall down if the get-device-token request happens to complete after the await _registerNotificationToken() (above) is called. That's because of this listener we add:

NotificationService.instance.token.addListener(_registerNotificationToken);

With that, it looks like we'll be sure to dispatch the register-with-server request as soon as we have a good result from the get-device-token request, which is good.

(And that behavior is covered in the tests; nice!)

Still, although that race is handled for users' benefit, I wonder if _registerNotificationToken and its public wrapper registerNotificationToken have an unnecessarily misleading return type. I think their callers can't tell by awaiting the Future whether the token registration succeeded, failed (as with an API error), or is still pending. Is that right? Maybe void as a return type is a better match for that behavior.

Maybe void is the right return type given that.

I see that the tests use the Future<void> return type by awaiting the returned Future. Instead of doing that, I wonder if e.g. FakeAsync.elapse could be used instead?

Yeah, this return type actually was void in a draft before I wrote the tests.

Instead of doing that, I wonder if e.g. FakeAsync.elapse could be used instead?

It's a bit tricky because in the "initially unknown" test case, we first want to wait for store.registerNotificationToken to complete whatever it's going to do, then check no request was made, and only after that wait for NotificationService.start to finish, including its getToken call.

I think a fully robust solution probably involves extending TestZulipBinding so that the test can control when getToken finishes.

Logging might be right for now, but I wonder about:

• retrying
• preparing to warn the user that setup failed or is not complete

I think this request will naturally fail in ordinary circumstances, like no Internet connection, no Google Play Services, etc., right?

I assume it might also hang indefinitely in some failure modes.

I think those could be TODOs for now though.

Yeah, when it fails we'll want to note that for showing a warning (as in #323). Which, it's true, TODO(log) doesn't cover.

I assume it might also hang indefinitely in some failure modes.

Yeah, could be. We'll want #323 or related warnings to appear for the user if we haven't gotten a result here.

One thing I meant to put in my review (just posted now) but forgot: why should test code want a LivePerAccountStore? 🙂

Well, to test registerNotificationToken. 🙂 Or poll — we don't have tests for that, but probably should, and there's ways it should get more complex than it is and then those should definitely get tested.

This does mean that those two methods make LivePerAccountStore not really analogous to LiveZulipBinding or ApiConnection.live — unlike those, it contains nontrivial logic of its own that isn't just integrating with external side effects outside of Dart, logic that is amenable to unit tests. That definitely suggests there's some kind of refactoring to be done here that would draw the lines more cleanly.

Ah yeah I guess that makes sense.